Skip to content

chore: Refactor task schedulers to use context.Context for lifecycle management#7746

Merged
Shaddoll merged 1 commit intocadence-workflow:masterfrom
Shaddoll:scheduler
Feb 25, 2026
Merged

chore: Refactor task schedulers to use context.Context for lifecycle management#7746
Shaddoll merged 1 commit intocadence-workflow:masterfrom
Shaddoll:scheduler

Conversation

@Shaddoll
Copy link
Member

@Shaddoll Shaddoll commented Feb 25, 2026 •
edited
Loading

What changed?
Refactor task schedulers to use context.Context instead of a channel for lifecycle management.

Why?
context.Context is the better default for lifecycle management. It supports propagation, deadline/timeout and is the standard convention in Go.

How did you test it?
unit tests
cd common/task && go test ./...

Potential risks
N/A

Release notes
N/A

Documentation Changes
N/A

Reviewer Validation

PR Description Quality (check these before reviewing code):

  • "What changed" provides a clear 1-2 line summary
    • Project Issue is linked
  • "Why" explains the full motivation with sufficient context
  • Testing is documented:
    • Unit test commands are included (with exact go test invocation)
    • Integration test setup/commands included (if integration tests were run)
    • Canary testing details included (if canary was mentioned)
  • Potential risks section is thoughtfully filled out (or legitimately N/A)
  • Release notes included if this completes a user-facing feature
  • Documentation needs are addressed (or noted if uncertain)

@gitar-bot
Copy link

gitar-bot bot commented Feb 25, 2026 •
edited
Loading

🔍 CI failure analysis for ec19356: Two activity heartbeat integration tests are consistently failing across multiple CI runs: TestActivityHeartbeatTimeouts and TestActivityHeartbeatDetailsDuringRetry. The failures reproduce consistently, indicating a regression related to the task scheduler refactoring.

Issue

Two activity heartbeat-related integration tests are consistently failing across multiple CI runs:

  1. TestActivityHeartbeatTimeouts - Expected activities to timeout but they completed successfully instead

    • Error at activity_test.go:1107: "Expected activities to timeout but seeing completion instead"
    • Test duration: ~18-19s (consistent across runs)
    • Job IDs: 64898331275, 64902727411

  2. TestActivityHeartbeatDetailsDuringRetry - Wrong retry count observed

    • Error at activity_test.go:317: Expected retry count of 2 but got 1
    • Test duration: ~61s (consistent across runs)
    • Job IDs: 64898331275, 64902727411

Root Cause

The consistent reproduction of these failures indicates this is likely a genuine regression caused by the task scheduler refactoring, not test flakiness.

The PR refactors task schedulers in common/task/ to use context.Context instead of channels:

  • Changed files: fifo_task_scheduler.go, weighted_round_robin_task_scheduler.go
  • Changes: Replaced shutdownCh chan struct{} with ctx context.Context and cancel context.CancelFunc
  • Behavior change: Using ctx.Done() instead of <-shutdownCh

These task schedulers are used by the history service task processor (service/history/task/processor.go), which handles activity timer tasks. The timing difference between context cancellation and channel closure may be affecting:

  • Activity heartbeat timeout detection
  • Task processing timing in transfer/timer queues
  • Activity retry scheduling

Details

Why context.Context might behave differently:

  1. Context cancellation propagation: When cancel() is called, it marks the context as done and closes an internal channel. This may have microscopically different timing characteristics than directly closing a channel.

  2. Task queue processing: The weighted round robin task scheduler is used in history service task processing. Any timing difference in shutdown/cancellation could affect:

    • When tasks are drained from queues
    • When activity timeout tasks are processed
    • The order of task processing during concurrent operations

  3. Activity heartbeat behavior: The tests specifically verify timing-sensitive behavior:

    • TestActivityHeartbeatTimeouts: Expects activities to timeout after heartbeat intervals expire
    • TestActivityHeartbeatDetailsDuringRetry: Expects specific retry counts based on heartbeat failure detection

If task processing is even slightly delayed or accelerated, it could cause:

  • Activities to complete before timeout detection triggers (Test 1)
  • Retry logic to execute fewer times than expected (Test 2)
Code Review ✅ Approved

Clean mechanical refactoring replacing shutdownCh with context.Context. All changes are semantically equivalent, idiomatic Go, and well-tested.

Rules ❌ No requirements met

Repository Rules

❌ PR Description Quality Standards: Expand Why section to explain previous shutdownCh implementation and its limitations; add -v flag to test command

2 rules not applicable. Show all rules by commenting gitar display:verbose.

Tip

Comment Gitar fix CI or enable auto-apply: gitar auto-apply:on

Options

Auto-apply is off → Gitar will not commit updates to this branch.
Display: compact → Showing less information.

Comment with these commands to change:

Auto-apply Compact 
gitar auto-apply:on         

gitar display:verbose

Was this helpful? React with 👍 / 👎 | Gitar

@Shaddoll Shaddoll enabled auto-merge (squash) February 25, 2026 21:18
@Shaddoll Shaddoll merged commit d52ae40 into cadence-workflow:master Feb 25, 2026
75 of 77 checks passed
@Shaddoll Shaddoll deleted the scheduler branch February 25, 2026 23:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@abhishekj720 abhishekj720 abhishekj720 approved these changes

@fimanishi fimanishi fimanishi approved these changes

@adhityamamallan adhityamamallan Awaiting requested review from adhityamamallan adhityamamallan is a code owner

@macrotim macrotim Awaiting requested review from macrotim macrotim is a code owner

@Assem-Uber Assem-Uber Awaiting requested review from Assem-Uber Assem-Uber is a code owner

@bowenxia bowenxia Awaiting requested review from bowenxia bowenxia is a code owner

@timl3136 timl3136 Awaiting requested review from timl3136 timl3136 is a code owner

@eleonoradgr eleonoradgr Awaiting requested review from eleonoradgr eleonoradgr is a code owner

@gazi-yestemirova gazi-yestemirova Awaiting requested review from gazi-yestemirova gazi-yestemirova is a code owner

@arzonus arzonus Awaiting requested review from arzonus arzonus is a code owner

@c-warren c-warren Awaiting requested review from c-warren c-warren is a code owner

@natemort natemort Awaiting requested review from natemort natemort is a code owner

@neil-xie neil-xie Awaiting requested review from neil-xie neil-xie is a code owner

@davidporter-id-au davidporter-id-au Awaiting requested review from davidporter-id-au davidporter-id-au is a code owner

@shijiesheng shijiesheng Awaiting requested review from shijiesheng shijiesheng is a code owner

@jakobht jakobht Awaiting requested review from jakobht jakobht is a code owner

@sankari165 sankari165 Awaiting requested review from sankari165 sankari165 is a code owner

@dkrotx dkrotx Awaiting requested review from dkrotx dkrotx is a code owner

@demirkayaender demirkayaender Awaiting requested review from demirkayaender demirkayaender is a code owner

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@Shaddoll @abhishekj720 @fimanishi